feat: add Trainer control-plane metrics - #4190

We don't need to add this in the phase 1, right @robert-bell @abhijeet-dhumal ?

I'd be keen to keep this in phase 1. It gives admins info about job sizes which is useful for investing in optimising network configs.

Why do we need a separate informer handler @abhijeet-dhumal @robert-bell?
In JobSet, we directly call the metrics counter from the controller loop: https://github.com/kubernetes-sigs/jobset/blob/main/pkg/controllers/failure_policy.go#L432

Using the informer completely decouples the metrics instrumentation from the reconciliation logic keeping both clean and easier to evolve independently.

Do we prefer approach to decouple it?

Would love to hear feedback from the Kueue maintainer on this too @kannon92 @tenzen-y!
https://github.com/kubernetes-sigs/kueue/tree/main/pkg/metrics

I prefer keeping the informer-based observer... it keeps metrics independent of reconciliation and records lifecycle events without adding metrics work to the reconcile path.

@abhijeet-dhumal we talked about this today at the Trainer call with @robert-bell and @tenzen-y.
Ref: https://youtu.be/nsE0a4T9LFM?t=959

Given the trade-offs of using separate watchers for events, could we explore how other Kubernetes projects handle event processing, particularly since our current implementation differs from Kueue and Training Operator v1?

My main concern with your proposed approach is that metric updates would rely solely on eventual consistency, as the metrics might not be updated at the time the corresponding event is processed.

Hey @andreyvelich, @tenzen-y, I've done an investigation into this.

I looked back at the call and I think there were 2 points we wanted to clarify:

1. What's the existing precedent for using the informer-cache pattern, rather than instrumenting the reconcile loop like kueue and Training Operator v1?
2. Are there any problems with the eventual consistency of the informer-backed metrics? Yuki's concern was that metrics recorded at cache-event time rather than controller-action time would diverge from the controller logs, making it harder for admins to correlate the two when debugging production issues.

How other projects do it.
The pattern in this PR has more precedent than the Kueue/Training Operator v1 comparison suggests:

• Kueue records finished_workloads_total from a watch event handler on the shared informer cache, with old/new transition guards (workload_controller.go, handleUpdate) -- structurally the same approach as this PR's terminal counters. Its scheduler-decision metrics are recorded at the decision point, which is the other category.
• Several widely-deployed projects derive object-state metrics from informer-backed caches, decoupled from reconcile loops:
  • kube-state-metrics: reflectors feed watch events into a MetricsStore served on scrape (internal/store/builder.go)
  • cert-manager: prometheus.Collectors over informer-backed listers (internal/collectors/certificate_collector.go); migrated to this pattern in v1.19 to fix stale-series bugs in its controller-instrumented metrics (#7301)
  • KubeVirt: collect-callback reads the VMI informer store at scrape time (vmistats_collector.go)
  • Argo Rollouts: lister-backed argo_rollouts_info collectors alongside reconcile counters in the same registry (controller/metrics/metrics.go)
  • KubeRay: Collect() lists RayJob/RayCluster objects through the manager's cached client (ray_job_metrics.go)
  • prometheus-adapter: metadata informers/listers back its entire serving path (cmd/adapter/adapter.go)
• JobSet and Training Operator v1 do count in the reconcile loop, and JobSet's own code comments document working around retry-driven "metric inflation" in that design.

The distinction that falls out is state-derived vs action-derived metrics. All the metrics in this PR are state metrics (what exists / what happened in the cluster), which is what the informer pattern is for; controller-behaviour metrics are already exported by controller-runtime's built-in reconcile_* family.

Eventual consistency in practice.
This PR is using the same cache as the main reconciliation loop, so there's no realistic divergence from what the reconcile loop would see. I did measure latency locally on a kind cluster which confirms the delay is minimal - e.g. trainjob_failed_total incremented within ~14 ms of an independent watch stream observing the terminal condition -- in some trials before my own watcher received the event.

@tenzen-y on your log-correlation concern: counter samples are timestamped at scrape time, so no user can localize a counter increment finer than the scrape interval (e.g. 30 s default) regardless of where the increment happens -- moving it into the reconcile loop would not change observable timing.

Recommendation
I think we should be good to go ahead with the implementation as-is -- though we can add an alpha feature gate for it if you'd prefer.

We do need to update docs/operator-guides/metrics.md to mention the metric collections are best effort. @abhijeet-dhumal could you add that to this PR when you get a chance? I'll add some suggestions.

For transparency, I used AI to help me with the investigation, but I've manually reviewed and edited the information.

I think we could add this clarification to help users understand the limitation -

Suggest we add this clarification